feat: Make toolbox & flyout properly focusable. #225

BenHenning · 2025-02-21T00:39:22Z

Fixes #180
Fixes #149
Fixes part of #101

The toolbox already had a tab stop, but it wouldn't become properly enabled for keyboard navigation. Now it is properly enabled and works when focused both via tab navigation and with the 'T' shortcut.

This has fairly substantial behavioral differences now:

Focus has been reworked to logically support either the toolbox or the workspace currently having DOM focus in order to support the toolbox properly having keyboard navigation support when enabled.
Keyboard navigation provided by core Blockly for toolbox has been monkey patched out since it would otherwise interfere with the toolbox navigation (and results in items being skipped when navigating through the categories).
The navigation controller now has support for a 'nowhere' state to support blurring the toolbox.
It's possible for the toolbox to have keyboard input with or without DOM focus which can result in some quirkiness when tabbing between the two--this may be something to iterate on if it introduces any user confusion.

From a long-term perspective, we may want to:

Limit some shortcuts to only cases when the workspace is focused (there's a behavior difference with either approach compared to the currently implementation).
Figure out a good way to ensure that these behaviors are properly captured in core Blockly as this plugin is moved over.

Separately, note that:

Per feedback from @rachel-fenichel this only handles toolbox focus and ignores flyouts altogether. Proper flyout support can be added later as needed.
A subcategory was added to the demo toolbox categories as part of the analysis from Toolbox with categories takes focus but doesn't indicate it or have working nav #149 (see the second video below).

The new behavior can be seen here:

Screen.recording.2025-03-11.4.59.00.PM.webm

Note that the specific behaviors to observe are around how tabbing to the toolbox allows it to be focused and have keyboard navigation enabled. The video also demonstrates some of the awkward behaviors of switching focus between the toolbox and workspace when the other one is actually focused (such as when inserting a block from a focused toolbox or using the 't' command from an active workspace).

Note that this video also shows how subcategories are currently a bit buggy with left/right navigation (an existing issue now being tracked in #241):

Screen.recording.2025-02-24.4.34.35.PM.webm

The toolbox already had a tab stop, but it wouldn't become properly enabled for keyboard navigation. Now it is properly enabled and works when focused both via tab navigation and with the 'T' shortcut.

BenHenning · 2025-02-21T00:43:10Z

@RoboErikG PTAL for approval.

@rachel-fenichel could you PTAL for the behavior in the video to make sure it looks good? I plan to ask @microbit-matt-hillsdon to take a look at this once it's merged, as well, to ensure it has good compatibility with MakeCode's toolbox.

BenHenning · 2025-02-21T01:45:25Z

TODO: Self, double check if ||'s should be replaced with ??'s.

microbit-matt-hillsdon · 2025-02-21T12:19:21Z

Interestingly this changes the behaviour for #222 and the flyout now switches from Variables to the first category after creating a variable.

We're a little worried this one might be hard for us to integrate ahead of user testing due to the toolbox interactions and we don't want to lose the ability to take other changes. We'll explore that today and get back to you.

microbit-matt-hillsdon · 2025-02-21T14:55:45Z

We're a little worried this one might be hard for us to integrate ahead of user testing due to the toolbox interactions and we don't want to lose the ability to take other changes. We'll explore that today and get back to you.

We've since done a test merge of this change and it's neutral from the perspective of the MakeCode toolbox.

I was hopeful that if the flyout learnt to take focus we could simplify some of what we've done there. At the moment when we hand off from the MakeCode toolbox to the flyout (right arrow) we DOM focus the workspace and call navigation.focusFlyout so that we have working keyboard nav in the flyout. It feels weird to me to see a focus outline on the workspace due to that. Long term I was hoping that could just be us calling focus on the flyout. I think it could be useful for us to discuss the MakeCode toolbox hack we have now in the light of the plan on #142.

RoboErikG · 2025-02-21T20:45:59Z

I'm not sure I understand the keyboard navigation pieces enough yet to do a quick review. Reassigning in case there's time pressure to get this in.

rachel-fenichel

This makes sense to me logically.

In the screencast I see that the text on all blocks in the flyout is highlighted as though selected. Please fix.

src/index.ts

src/navigation_controller.ts

src/navigation.ts

src/navigation_controller.ts

src/index.ts

This really helps with the route to the MakeCode sim and matches the behaviour on RaspberryPiFoundation#225

Conflicts: src/navigation_controller.ts

BenHenning · 2025-02-25T00:58:11Z

We're a little worried this one might be hard for us to integrate ahead of user testing due to the toolbox interactions and we don't want to lose the ability to take other changes. We'll explore that today and get back to you.

We've since done a test merge of this change and it's neutral from the perspective of the MakeCode toolbox.

I was hopeful that if the flyout learnt to take focus we could simplify some of what we've done there. At the moment when we hand off from the MakeCode toolbox to the flyout (right arrow) we DOM focus the workspace and call navigation.focusFlyout so that we have working keyboard nav in the flyout. It feels weird to me to see a focus outline on the workspace due to that. Long term I was hoping that could just be us calling focus on the flyout. I think it could be useful for us to discuss the MakeCode toolbox hack we have now in the light of the plan on #142.

Thanks for checking @microbit-matt-hillsdon. Just to clarify: is there anything else you specifically need here ahead of the user test, or do you think the changes here are sufficient & compatible?

Re: the focus outline, I agree completely. It's confusing that the toolbox can hold focus but the workspace be receiving input and vice versa (both of which states are now made possible with this PR). The planned changes in #142 should fix that, though there will need to be a fair amount of reworking of the plugin to rely more on focus and less on presuming focus (i.e. Blockly elements need to actually have focus not just the indicator), but that's expected as part of the broader focus work.

This helps to verify subcategory behavior (which has been observed as not completely working currently).

BenHenning

Self-reviewed latest changes.

Thanks @rachel-fenichel. PTAL.

rachel-fenichel

LGTM. Thanks for simplifying out the flyout case for now--please also file a bug to follow up on the flyout case, if we don't already have one.

src/navigation.ts

src/toolbox_monkey_patch.js

microbit-matt-hillsdon · 2025-02-25T09:54:47Z

Thanks for checking @microbit-matt-hillsdon. Just to clarify: is there anything else you specifically need here ahead of the user test, or do you think the changes here are sufficient & compatible?

There's nothing further we need here, thanks. If it is going to be merged before UT then the sooner the better from our point of view. We can live without it if needed just because the MakeCode toolbox already had these behaviours (and we've aligned them where this PR differed).

This really helps with the route to the MakeCode sim and matches the behaviour on RaspberryPiFoundation#225

Conflicts: src/navigation_controller.ts

BenHenning · 2025-03-06T00:18:20Z

Thanks @rachel-fenichel. I followed up with a fix for the monkey patch installation--any concerns? It seems a bit awkward to me due to how early in the application lifecycle it needs to be 'installed'.

Re two earlier comments:

"please also file a bug to follow up on the flyout case, if we don't already have one." -- which flyout case do you mean?
"In the screencast I see that the text on all blocks in the flyout is highlighted as though selected." -- is this the flyout case? Regardless, I'm not sure that I fully understand what's observed as broken. Approximately which part in the video do you see the broken behavior (I admittedly should have slowed it down originally since it navigates through parts of the UI a bit quickly).

rachel-fenichel · 2025-03-06T18:00:02Z

The text on the blocks (if, do, etc) looks like it has been selected with the mouse on all blocks in the flyout. It looks like that's true for all categories.

Also I shouldn't have merged my change before yours--sorry about the merge conflict >.<

test/index.ts

rachel-fenichel · 2025-03-07T00:21:26Z

"the flyout case" meaning the case that there is a flyout with no toolbox.

Conflicts: src/navigation_controller.ts

It seems possible now to call hide() earlier than previously expected (i.e. when there's a cursor but no current node, such as in the case of focusing the toolbox before the workspace as this PR now makes possible).

BenHenning

Self-reviewed latest changes.

@rachel-fenichel thanks for the extra details on the text issue. It seems that it mysteriously went away after syncing to latest, so that's fun. :) If you could, PTAL at the latest changes just to make sure everything seems reasonable. I've enabled auto-merge so feel free to merge this, as well, if there are no follow-ups. The first video in the description has also been updated accordingly.

Dismissing so that I can enable auto-merge.

BenHenning · 2025-03-12T00:04:41Z

Hmm GitHub is very mysterious. I apparently don't need to get re-approval after dismissing approval to merge. Well, please go ahead and merge this upon approval if it looks good @rachel-fenichel otherwise I will tomorrow AM.

rachel-fenichel · 2025-03-12T00:05:35Z

Changes LGTM.

* feat: Make toolbox & flyout properly focusable. The toolbox already had a tab stop, but it wouldn't become properly enabled for keyboard navigation. Now it is properly enabled and works when focused both via tab navigation and with the 'T' shortcut. * chore: address reviewer comments * feat: add subcategory to demo toolbox categories This helps to verify subcategory behavior (which has been observed as not completely working currently). * Address reviewer comment for monkey patch install. * Revert monkeypatch installation changes. * Add fix for null current node. It seems possible now to call hide() earlier than previously expected (i.e. when there's a cursor but no current node, such as in the case of focusing the toolbox before the workspace as this PR now makes possible).

feat: Make toolbox & flyout properly focusable.

6935180

The toolbox already had a tab stop, but it wouldn't become properly enabled for keyboard navigation. Now it is properly enabled and works when focused both via tab navigation and with the 'T' shortcut.

BenHenning marked this pull request as ready for review February 21, 2025 00:41

BenHenning requested a review from a team as a code owner February 21, 2025 00:41

BenHenning requested review from RoboErikG and removed request for a team February 21, 2025 00:41

BenHenning assigned RoboErikG and rachel-fenichel Feb 21, 2025

RoboErikG requested a review from rachel-fenichel February 21, 2025 20:45

rachel-fenichel reviewed Feb 22, 2025

View reviewed changes

microbit-matt-hillsdon mentioned this pull request Feb 24, 2025

Close flyout when navigating from toolbox microbit-matt-hillsdon/blockly-keyboard-experimentation#2

Merged

BenHenning mentioned this pull request Feb 24, 2025

Toolbox with categories takes focus but doesn't indicate it or have working nav #149

Closed

BenHenning added 2 commits February 25, 2025 00:22

chore: address reviewer comments

d05c741

Merge branch 'main' into make-toolbox-properly-focusable

ee21ee2

Conflicts: src/navigation_controller.ts

BenHenning mentioned this pull request Feb 25, 2025

Left/right navigation doesn't quite work correctly with subcategories #241

Open

feat: add subcategory to demo toolbox categories

dabb9d9

This helps to verify subcategory behavior (which has been observed as not completely working currently).

BenHenning commented Feb 25, 2025

View reviewed changes

rachel-fenichel approved these changes Feb 25, 2025

View reviewed changes

src/navigation.ts Show resolved Hide resolved

src/toolbox_monkey_patch.js Show resolved Hide resolved

RoboErikG removed their request for review February 27, 2025 22:50

BenHenning added 2 commits March 6, 2025 00:12

Address reviewer comment for monkey patch install.

1a8d972

Merge branch 'main' into make-toolbox-properly-focusable

e27aa11

Conflicts: src/navigation_controller.ts

rachel-fenichel previously approved these changes Mar 6, 2025

View reviewed changes

test/index.ts Outdated Show resolved Hide resolved

BenHenning added 3 commits March 11, 2025 23:45

Merge branch 'main' into make-toolbox-properly-focusable

77ae347

Conflicts: src/navigation_controller.ts

Revert monkeypatch installation changes.

6ec8ceb

Add fix for null current node.

2f41d8f

It seems possible now to call hide() earlier than previously expected (i.e. when there's a cursor but no current node, such as in the case of focusing the toolbox before the workspace as this PR now makes possible).

BenHenning commented Mar 12, 2025

View reviewed changes

BenHenning requested a review from rachel-fenichel March 12, 2025 00:03

BenHenning unassigned RoboErikG Mar 12, 2025

rachel-fenichel approved these changes Mar 12, 2025

View reviewed changes

BenHenning merged commit 6327ca7 into RaspberryPiFoundation:main Mar 12, 2025
3 checks passed

BenHenning deleted the make-toolbox-properly-focusable branch March 12, 2025 00:05

feat: Make toolbox & flyout properly focusable. #225

feat: Make toolbox & flyout properly focusable. #225

Uh oh!

Conversation

BenHenning commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented Feb 21, 2025

Uh oh!

BenHenning commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

microbit-matt-hillsdon commented Feb 21, 2025

Uh oh!

microbit-matt-hillsdon commented Feb 21, 2025

Uh oh!

RoboErikG commented Feb 21, 2025

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenHenning commented Feb 25, 2025

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

microbit-matt-hillsdon commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenHenning commented Mar 6, 2025

Uh oh!

rachel-fenichel commented Mar 6, 2025

Uh oh!

Uh oh!

rachel-fenichel commented Mar 7, 2025

Uh oh!

BenHenning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Mar 12, 2025

Uh oh!

rachel-fenichel commented Mar 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BenHenning commented Feb 21, 2025 •

edited

Loading

BenHenning commented Feb 21, 2025 •

edited

Loading

microbit-matt-hillsdon commented Feb 25, 2025 •

edited

Loading

BenHenning left a comment •

edited

Loading